Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed bug with hierarchical vocabularies #4048

Merged
merged 1 commit into from
Sep 5, 2020

Conversation

daguiler
Copy link
Contributor

@daguiler daguiler commented Sep 3, 2020

Fixes #4047

Summary

Several problems in this findInChildren function.

  1. The forEach loop cannot be broken, so the return statement at line 19 was not actually exiting the function.
  2. The initial value of parentTerm was an empty object, so if the item was not found, an empty object was being returned by the function. However, empty objects evaluate to true, not to false.
  3. Since this is a recursive function, and because of 2), line 21 was deciding to continue instead of deciding to stop. This was causing the parentTerm variable to get incorrectly overwritten in following cycles.

I replaced this buggy function with a more clear implementation that immediately breaks the loop if the item is found, or returns null otherwise.

@bdukes
Copy link
Contributor

bdukes commented Sep 4, 2020

Your description states that your new function "immediately breaks the loop if the item is found", but I don't see an immediate return. Other than adding a return, this looks good to me. Thanks!

@bdukes bdukes added this to the 9.7.2 milestone Sep 4, 2020
@daguiler
Copy link
Contributor Author

daguiler commented Sep 4, 2020

Hi @bdukes, the break is in the for condition.

Copy link
Contributor

@valadas valadas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@valadas valadas merged commit fe92fc2 into dnnsoftware:develop Sep 5, 2020
@daguiler daguiler deleted the bugfix/DNN-40577 branch September 5, 2020 01:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug when adding terms to a hierarchical vocabulary
3 participants